Skip to content

python, swig: fix bugs and restore Python ergonomics after libnvmf_context refactor#3291

Closed
martin-belanger wants to merge 3 commits intolinux-nvme:masterfrom
martin-belanger:swig-python-bindings-fix
Closed

python, swig: fix bugs and restore Python ergonomics after libnvmf_context refactor#3291
martin-belanger wants to merge 3 commits intolinux-nvme:masterfrom
martin-belanger:swig-python-bindings-fix

Conversation

@martin-belanger
Copy link
Copy Markdown

Three commits fixing regressions and compatibility issues introduced when the C API moved from libnvme_fabrics_config to the opaque libnvmf_context.

Commit 1 — Py_NewRef compatibility (Python < 3.10)
SWIG ≥ 4.1 generates calls to Py_NewRef(), which was added in Python 3.10. Distributions with SWIG ≥ 4.1 and Python < 3.10 (e.g. SLES 15.x) got an undefined symbol at import time. Fixed with a %begin compat shim and a meson warning when the condition is detected.

Commit 2 — Bug fixes from the libnvmf_context refactor

  • %pythonappend for connect() had a stale fctx parameter — SWIG silently ignored it, so the GC guard (self.__host = h) never ran, leaving a latent use-after-free.
  • duplicate_connect check was inverted in the SWIG binding. Moved to the C library (libnvmf_add_ctrl()) where it belongs, with the correct polarity.
  • fctx_set_fabrics_config was missing its %rename, exposing it to Python with the ugly fctx_ prefix intact.

Commit 3 — Restore Python ergonomics: dict-based ctrl constructor
The refactor forced Python callers to manually create, configure, and discard a libnvmf_context — a C implementation detail with no value at the Python level. This commit hides it entirely behind a %typemap(in/freearg) pair that converts a Python dict to an fctx transparently. The typemap is reusable for any future function that takes struct libnvmf_context *.

The dict covers all configurable fields: connection identity, the full libnvme_fabrics_config (set via the generated accessors), host identity, DH-HMAC-CHAP keys, TLS parameters, and persistence. help(nvme.ctrl) documents every supported key via an autodoc string.

Martin Belanger added 3 commits April 20, 2026 12:21
SWIG >= 4.1 generates calls to Py_NewRef() in its runtime boilerplate.
Py_NewRef was introduced in Python 3.10, causing an undefined symbol
error on older distributions (e.g. SLES 15.6/15.7 with Python 3.6).

The shim must live in a %begin block so it precedes SWIG's own runtime
in the generated C file. Since Python.h is not yet included at that
point, PyObject is unavailable, so a macro is used instead of an inline
function. A -DSWIG_COMPAT_PY_NEWREF flag, set by meson when Py_NewRef
is absent, gates the macro so it never conflicts with Python 3.10+'s own
definition.

Signed-off-by: Martin Belanger <[email protected]>
Assisted-by: Claude Sonnet 4.6 <[email protected]>
Three bugs were introduced when libnvmf_context replaced the old
libnvme_fabrics_config parameter in the connect/create flow:

1. The %pythonappend GC guard for connect() had a stale signature that
   still included the removed fctx parameter. SWIG silently ignores a
   mismatched %pythonappend, so self.__host = h never ran, leaving a
   latent use-after-free where Python could collect the host object
   while the ctrl was still alive.

2. The duplicate_connect check in connect() had an inverted condition
   (missing !) and belonged in the library, not the binding. Move it
   to libnvmf_add_ctrl() in fabrics.c where it applies uniformly to
   all callers, C and Python alike.

3. fctx_set_fabrics_config was missing its %rename directive, exposing
   it to Python as fctx.fctx_set_fabrics_config() instead of
   fctx.set_fabrics_config() like all other libnvmf_context methods.

Signed-off-by: Martin Belanger <[email protected]>
Assisted-by: Claude Sonnet 4.6 <[email protected]>
The libnvmf_context object was a leaky abstraction: Python callers had to
create it, configure it, pass it to ctrl(), then discard it — despite never
needing it after construction. This commit restores the ergonomics that were
lost when the C API switched from libnvme_fabrics_config to libnvmf_context.

The public header declares libnvmf_context as an opaque forward declaration,
which caused SWIG to treat it as an opaque pointer type and override any
user-defined %typemap. Fix this by providing an empty struct body in the SWIG
parsed section, which gives SWIG a complete type and restores normal typemap
precedence without modifying any C header.

The %typemap(in) / %typemap(freearg) pair for struct libnvmf_context * now
handles the full fctx lifecycle transparently: the typemap creates the fctx
from the Python dict (using arg1 as the global context), the constructor body
calls libnvmf_create_ctrl(), and freearg frees the fctx. The typemap is
reusable for any future function that takes struct libnvmf_context *.

The dict supports all configurable fields: connection identity (subsysnqn,
transport, traddr, trsvcid, host_traddr, host_iface), the full
libnvme_fabrics_config (via libnvme_fabrics_config_set_* accessors and
libnvmf_context_get_fabrics_config), host identity (hostnqn, hostid), crypto
and TLS (hostkey, ctrlkey, keyring, tls_key, tls_key_identity), and
persistence. All context-level fields are set via their public API setters.

An autodoc string on the constructor documents every supported key so that
help(nvme.ctrl) is the complete reference. Tests are updated to use the new
dict-based API.

Signed-off-by: Martin Belanger <[email protected]>
Assisted-by: Claude Sonnet 4.6 <[email protected]>
@martin-belanger
Copy link
Copy Markdown
Author

Had to redo this. Closing.

@martin-belanger martin-belanger deleted the swig-python-bindings-fix branch April 21, 2026 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant